test: reduce testnode overrides #4066
Conversation
rootulp
requested review from
cmwaters and
staheri14
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request introduces a new function, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
test/util/testnode/comet_node_test.go (1)
46-50: Replace magic number with a named constantThe 200 MiB value should be defined as a constant with a meaningful name to improve maintainability and clarity.
+const maxRPCBodyBytes = 200 * mebibyte // 200 MiB for large transactions and responses func customTendermintConfig() *tmconfig.Config { // ... - tmCfg.RPC.MaxBodyBytes = 200 * mebibyte + tmCfg.RPC.MaxBodyBytes = maxRPCBodyBytestest/util/testnode/config.go (1)
Line range hint
147-152: Consider documenting the timeout commit configuration.While the 1ms timeout commit is retained for faster test execution, it would be helpful to add a comment explaining why this differs from mainnet configuration, as it's one of the few remaining overrides.
func DefaultTendermintConfig() *tmconfig.Config { tmCfg := tmconfig.DefaultConfig() + // Note: This timeout differs from mainnet configuration to speed up test execution. + // This is an acceptable override as it doesn't affect the functional behavior being tested. // Reduce the timeout commit to 1ms to speed up the rate at which the test // node produces blocks. tmCfg.Consensus.TimeoutCommit = 1 * time.Millisecond
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
test/util/testnode/comet_node_test.go(3 hunks)test/util/testnode/config.go(1 hunks)
🔇 Additional comments (3)
test/util/testnode/comet_node_test.go (3)
19-19: LGTM: Import addition is appropriate
The new import is necessary for the configuration customization and follows Go naming conventions.
43-44: LGTM: MaxTxBytes configuration aligns with PR objectives
The override for MaxTxBytes is well-documented and correctly uses the appconsts.DefaultUpperBoundMaxBytes constant.
65-66: LGTM: Test setup changes are appropriate
The modifications correctly integrate the blob parameters and custom Tendermint configuration, aligning with the PR objective of handling specific test requirements at the individual test level.
| func customTendermintConfig() *tmconfig.Config { | ||
| tmCfg := DefaultTendermintConfig() | ||
| // Override the mempool's MaxTxBytes to allow the testnode to accept a | ||
| // transaction that fills the entire square. Any blob transaction larger | ||
| // than the square size will still fail no matter what. | ||
| maxTxBytes := appconsts.DefaultUpperBoundMaxBytes | ||
| tmCfg.Mempool.MaxTxBytes = maxTxBytes | ||
|
|
||
| // Override the MaxBodyBytes to allow the testnode to accept very large | ||
| // transactions and respond to queries with large responses (200 MiB was | ||
| // chosen only as an arbitrary large number). | ||
| tmCfg.RPC.MaxBodyBytes = 200 * mebibyte | ||
|
|
||
| tmCfg.RPC.TimeoutBroadcastTxCommit = time.Minute | ||
| return tmCfg | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can now provide a final response since we have found the mainnet configuration values in app/default_overrides.go.
Adjust test configuration values to match mainnet settings
The test configuration values significantly differ from mainnet settings and should be aligned:
MaxBodyBytes: Test uses 200 MiB vs mainnet's 8 MiBTimeoutBroadcastTxCommit: Test uses 60s vs mainnet's 50s
Please update these values in customTendermintConfig() to match the mainnet configuration from app/default_overrides.go.
🔗 Analysis chain
Verify configuration values against mainnet
According to the PR objectives, these configurations should align with mainnet values. Let's verify these settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mainnet configuration references
rg -A 5 "MaxBodyBytes|TimeoutBroadcastTxCommit" --type go
# Search for any other test configurations
ast-grep --pattern 'func $_()*Config() *tmconfig.Config {
$$$
}'
Length of output: 2080
| // Set all the ports to random open ones. | ||
| tmCfg.RPC.ListenAddress = fmt.Sprintf("tcp://127.0.0.1:%d", mustGetFreePort()) | ||
| tmCfg.P2P.ListenAddress = fmt.Sprintf("tcp://127.0.0.1:%d", mustGetFreePort()) | ||
| tmCfg.RPC.GRPCListenAddress = fmt.Sprintf("tcp://127.0.0.1:%d", mustGetFreePort()) |
There was a problem hiding this comment.
💡 Codebase verification
Caution: Configuration removal requires test adjustments
The removed configurations are actively used in test files and serve specific purposes:
test/util/testnode/comet_node_test.gouses these settings to handle large transactions and responsestest/e2e/testnet/setup.goandtest/e2e/benchmark/benchmark.goconfigure mempool size for test scenarios
These test-specific configurations should be preserved or alternative approaches should be implemented to maintain test functionality.
🔗 Analysis chain
LGTM! Configuration simplification aligns with mainnet.
The removal of custom mempool and RPC configurations while keeping only the necessary network address settings aligns well with the PR objective of matching mainnet configuration. This change ensures that tests run with settings closer to production.
Let's verify that no other tests rely on the removed configurations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that might be affected by the removal of MaxTxBytes, MaxBodyBytes, and TimeoutBroadcastTxCommit configs
rg -l "MaxTxBytes|MaxBodyBytes|TimeoutBroadcastTxCommit" --type go test/
Length of output: 170
Script:
#!/bin/bash
# Let's check the actual usage of these configurations in the found files
echo "=== test/util/testnode/comet_node_test.go ==="
rg -A 5 "MaxTxBytes|MaxBodyBytes|TimeoutBroadcastTxCommit" test/util/testnode/comet_node_test.go
echo -e "\n=== test/e2e/testnet/setup.go ==="
rg -A 5 "MaxTxBytes|MaxBodyBytes|TimeoutBroadcastTxCommit" test/e2e/testnet/setup.go
echo -e "\n=== test/e2e/benchmark/benchmark.go ==="
rg -A 5 "MaxTxBytes|MaxBodyBytes|TimeoutBroadcastTxCommit" test/e2e/benchmark/benchmark.go
Length of output: 1674
Closes #3775
Motivation
The default testnode config should closely match the mainnet config. If we have one-off tests that require overrides for certain values then they should be overridden at the test level not at the default testnode config level.